-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Client notification support #1611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Client notification support #1611
Conversation
|
New commit does the following:
I'm still not hitting 100% test coverage, but I'll try to improve that in the next few days. |
|
From the discussion in the contributor's Discord There's a reason for the validation. The protocol defines the message types. Clients and servers are usually written by separate entities, and so a client isn't going to know your server's custom notification type or visa versa. Rather than push a PR to a specific SDK you're using in order to allow an out-of-spec notification pattern, if you feel strongly enough about it and can make an argument for it that could pass muster at a core maintainers meeting, the right approach is to create an SEP and state your case for changing the spec. Then all the SDKs must fall in line. |
|
@cliffhall the most recent commit removes the feature, I don't feel very strongly about it - I see pros and cons on both sides and personally lean towards not worrying about it right now, notifications aren't being heavily used it seems because before this PR clients built with SDK couldn't handle most supported notifications to begin with, and I have only seen a few complaints about this issue here and there. I don't see a need for changing the protocol before there's any clear community desire for it. I discovered the trick for custom notifications (like many actual features in the SDK) just by reading the code and playing with what seemed possible. The inspector being able to display these notifications hinted that it might be possible to support them clientside, but I didn't notice the validation until I was tightening up the initial set of tests for the feature. So now the PR just supports client-side handlers for the expected notification types defined in the protocol. |
@kylestratis You should probably tidy the language in the PR description to indicate that it's not adding support for custom notifications but instead supporting the protocol specified notification types that weren't supported before, i.e., removing mention of |
Done, thanks! |
This PR expands the universe of supported server notification types that a client can handle to include all existing types defined in
types.py.Motivation and Context
In the current state, Python clients can only handle a few notifications sent by servers, essentially making those server-sent notifications useless. To empower client developers to handle any kind of notification servers may send them, I created this PR.
How Has This Been Tested?
test_custom_notifications.pyandtest_notification_callbacks.pyBreaking Changes
None, this approach is fully backward-compatible
Types of changes
Checklist
Additional context
There are several ServerNotification types that aren't handled by the client, so Python clients are unable to handle them with callbacks. This adds support for notification-specific callbacks in the client session, all except the cancellation notification which is handled natively by the
BaseSession. The progress notification is also handled inBaseSessionbut in that case,_received_notification()is still called, so clients can use a callback to do additional handling of a progress notification if they wish.